Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better commit message for breaking changes. #16846

Closed
wants to merge 2 commits into from

Conversation

MatonAnthony
Copy link
Contributor

@MatonAnthony MatonAnthony commented Nov 6, 2017

In this change, I would like to propose to emphasize on the importance of a good commit message in general.
But also and especially on breaking changes to make the documentation process easier.

I think the readiness of the commit message and a proper and detailled content can make a whole world of difference to understand breaking changes and the reason behind them.

This suggestion comes from a Twitter discussion available here: https://twitter.com/MylesBorins/status/927625601112006656

Checklist
Affected core subsystem(s)
  • Documentation

PS: I am sorry, I'm proposing a change regarding commit messages and mine is awful, I did this on Github editor, I will rebase this commit to change its message at once.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 6, 2017
@Fishrock123
Copy link
Contributor

Fishrock123 commented Nov 6, 2017

For posterity, this is heavily influenced by my lost attempt at making a Node 7 to 8 breaking changes doc (https://github.com/nodejs/node/wiki/Breaking-changes-between-Node-7-and-8).

The changes are too many and too obscure for a single person (or likely, even a small group of people) to do at major releases. (Certainly, I didn't even bother for Node 9, which what, probably has even more changes??)

The purpose of such a doc is to allow people with fairly low knowledge of Node.js internals to look for something they are using and identify what might have changed in it's use.
As such the most important parts are:

  1. What is the exact behavior that changed?
  2. What Node API that users may use would run into that change?

An actual formatted item that could be directly inserted into the doc would be ideal, but even that info might've made my job feasible.

Add informations to CONTRIBUTING.md requesting the committers
and especially the one working on breaking changes to write
proper commits messages containing the reason behind the
breaking changes, the case during which it can be triggered,
and a description of the breaking change in itself.
@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Nov 6, 2017
CONTRIBUTING.md Outdated
@@ -334,6 +334,10 @@ use `Refs:`.
- `Refs: http://eslint.org/docs/rules/space-in-parens.html`
- `Refs: https://github.com/nodejs/node/pull/3615`

5. If your commit introduce a breaking change (`semver-major`), it should contain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/introduce/introduces

CONTRIBUTING.md Outdated
5. If your commit introduce a breaking change (`semver-major`), it should contain
an explanation about the reason of the breaking change, which situation would trigger
the breaking change and what is the exact change.

Copy link
Member

@fhinkel fhinkel Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding something like this:
Breaking changes will be listed in the wiki with the aim to make upgrading easier. Please have a look at Breaking Changes for the level of detail that's suitable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I will make a follow-up commit

- Fix a grammar mispelling
- Add a paragraph describing the use and the ideal commit messages.
@bnoordhuis
Copy link
Member

@Fishrock123 Can you give examples of PRs that don't already do that? Breaking changes get the semver-major tag and if there's one thing I've been hammering on all these years, it's that authors should write up good commit logs.

@MatonAnthony
Copy link
Contributor Author

@Fishrock123 It could be a good occasion to propose a parsable format for semver-major commit messages.

For example adding something like this after the description:

Changed-Behavior: Description of the change of behavior
Affected-Apis: Affected Apis by this change

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

As a side note: I've been considering that we should start adding the semver label of the commit to the commit metadata so that we do not always have to go looking up the original PR to find it.

@MatonAnthony
Copy link
Contributor Author

I can add this onto this PR if you want (which seems to be a clear gain for me, the ability to grep through the commit label could be kind of a game changer for CLI addict and automation)

@Fishrock123
Copy link
Contributor

Don't remember any examples, sorry. You'd have to look through it yourself.

@Fishrock123
Copy link
Contributor

Oh and uh, I don't think commit metadata is useful here. Proper notes in the PR should work fine.

@MatonAnthony
Copy link
Contributor Author

@Fishrock123 Isn't this maintaining a dependency to Github ?

@fhinkel
Copy link
Member

fhinkel commented Nov 11, 2017

@MatonAnthony Thanks so much for your first commit 🎉. I landed this as
2dcdd1d, if we want more changes we can iterate from here.

@fhinkel fhinkel closed this Nov 11, 2017
fhinkel pushed a commit that referenced this pull request Nov 11, 2017
Add informations to CONTRIBUTING.md requesting the committers
and especially the one working on breaking changes to write
proper commits messages containing the reason behind the
breaking changes, the case during which it can be triggered,
and a description of the breaking change in itself.

PR-URL: #16846
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Fishrock123
Copy link
Contributor

@MatonAnthony It is, but we already have that dependency (tags, discussions) so... yeah.

@refack
Copy link
Contributor

refack commented Nov 11, 2017

We should find a way to broadcast important changes, such as this.

@fhinkel
Copy link
Member

fhinkel commented Nov 12, 2017

@refack Yes, what do you suggest?

@joyeecheung
Copy link
Member

Should we have some kind of format for those descriptions? Maybe a bullet list following a common sentence like "this patch introduces breaking changes:"?

@refack
Copy link
Contributor

refack commented Nov 12, 2017

Yes, what do you suggest?

ping @nodejs/collaborators is a start (I was also thinking of a low-traffic mailing list, I'll open an issue #16963 )

Should we have some kind of format for those descriptions? Maybe a bullet list following a common sentence like "this patch introduces breaking changes:"?

Suggestion:

Breaking: XXXXXXXXXXX

where 50 < length(XXXXXXXXX) < 240 (in 72 char lines obv)

@sam-github
Copy link
Contributor

One thing to think about is sometimes its not known that a commit is broken until after it has landed, and sometimes we go back and forth about semver-majorness after the fact, so there is some advantage to leaving that info as mutable metadata aka github labels. But I agree that the fact that its not clear in the commit message is unfortunate.

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Add informations to CONTRIBUTING.md requesting the committers
and especially the one working on breaking changes to write
proper commits messages containing the reason behind the
breaking changes, the case during which it can be triggered,
and a description of the breaking change in itself.

PR-URL: #16846
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Add informations to CONTRIBUTING.md requesting the committers
and especially the one working on breaking changes to write
proper commits messages containing the reason behind the
breaking changes, the case during which it can be triggered,
and a description of the breaking change in itself.

PR-URL: #16846
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants